-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[JENKINS-70531] Apply timeout on WebSocket write operations #7596
Conversation
// As in Engine.runWebSocket: | ||
LOGGER.finest(() -> "sending message of length " + (headerAndData.remaining() - 2)); | ||
try { | ||
sendBinary(headerAndData).get(5, TimeUnit.MINUTES); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that any channel write operation should be under 5 minutes?
I know it's a bad practice, but what if a plugin is copying big files from the agent to the controller, that could easily take more than 5 minutes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean if you are using archiveArtifacts
without artifact-manager-s3
. Yes that could easily run over 5m for one copy operation; but this timeout is for one transport frame, which is currently only 8k (for WebSocket it could probably be increased considerably). Even one Command
(broken into frames) would not necessarily be the full file size, since you would be using a RemoteOutputStream
sending ProxyOutputStream.Chunk
s which default to 1Mb. (all IIUC, but certainly worth testing)
@jglick i know this is still in draft, but I was wondering, would this PR be considered for lts-backport? |
Perhaps. I added |
This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. |
…i#7596) * Apply timeout on WebSocket write operations * jenkinsci/remoting#621 released (cherry picked from commit e0aee59)
Matches jenkinsci/remoting#621. https://issues.jenkins.io/browse/JENKINS-70531
Testing done
See upstream.
Proposed changelog entries
Maintainer checklist
Before the changes are marked as
ready-for-merge
:upgrade-guide-needed
label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidate
to be considered (see query).